Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Force activator always in path when activator CA is specified #12790

Merged
merged 3 commits into from
Apr 2, 2022
Merged

Force activator always in path when activator CA is specified #12790

merged 3 commits into from
Apr 2, 2022

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Mar 28, 2022

As described in the feature doc of #11906,
activator needs to serve TLS traffic for the traffic from ingress.

So, this patch force SKS proxy mode when activator-ca in
config-network is specified.

Part of #11906

@knative-prow knative-prow bot added area/API API objects and controllers size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/autoscale approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 28, 2022
@nak3 nak3 mentioned this pull request Mar 28, 2022
Merged
7 tasks
@nak3 nak3 force-pushed the always-activator-path branch from dc4de02 to 1d5f022 Compare March 28, 2022 11:08
@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 28, 2022
@nak3
Force activator always in path when activator CA is specified
15845a5 
As described in the feature doc of knative#11906,
activator needs to serve TLS traffic for the traffic from ingress.

So, this patch force SKS proxy mode when `activator-ca` in
`config-network` is specified.
@nak3 nak3 force-pushed the always-activator-path branch from 1d5f022 to 15845a5 Compare March 28, 2022 11:13
@codecov
Copy link

codecov bot commented Mar 28, 2022 •
edited
Loading

Codecov Report

Merging #12790 (be76f3b) into main (ad64980) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #12790      +/-   ##
==========================================
+ Coverage   87.51%   87.54%   +0.02%     
==========================================
  Files         196      196              
  Lines        9782     9787       +5     
==========================================
+ Hits         8561     8568       +7     
+ Misses        934      932       -2     
  Partials      287      287
Impacted Files Coverage Δ
pkg/reconciler/autoscaling/config/store.go 100.00% <100.00%> (ø)
pkg/reconciler/autoscaling/kpa/kpa.go 95.23% <100.00%> (+0.11%) ⬆️
pkg/reconciler/configuration/configuration.go 84.61% <0.00%> (-1.54%) ⬇️
pkg/reconciler/revision/background.go 91.81% <0.00%> (+1.81%) ⬆️
pkg/autoscaler/statserver/server.go 79.61% <0.00%> (+1.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad64980...be76f3b. Read the comment docs.

@nak3
Copy link
Contributor Author

nak3 commented Mar 28, 2022

/cc @evankanderson @skonto @rhuss

@dprotaso
Copy link
Member

Do we need to force the activator to always be in the path?

I thought the ingress => queue proxy would also be encrypted too

@nak3
Copy link
Contributor Author

nak3 commented Mar 28, 2022 •
edited
Loading

Yes, it should be in the future. But for the initial Alpha release, we will force the activator to always be in the path.
We can refer to the feature docs:

the HTTP Proxy (KIngress) needs to be able to accept certificates for both the activator (in the knative-serving namespace) and the Revision pods (in the user namespace). There are three ways to manage this:
... snip ...
For the initial Alpha release, we will implement approach 2 and reduce scope by keeping the activator always (and only) in the set of Knative Revision endpoints exposed to the KIngress.

@evankanderson
Copy link
Member

I'm not sure if Dave is suggesting that we might want to extend the comment around line 135 in kpa.go. I think the link to the GitHub issue may be sufficient, though we should be clear somewhere that this is a temporary state, rather than the long-term goal.

evankanderson
evankanderson reviewed Mar 29, 2022
View reviewed changes
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

/hold

(for the typo and possibly other comment changes)

pkg/reconciler/autoscaling/kpa/kpa.go Outdated Show resolved Hide resolved
@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2022
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2022
@knative-prow knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2022
@nak3
Copy link
Contributor Author

nak3 commented Mar 30, 2022

Thank you! I opened #12797 and extended the comment with the link.

psschwei
psschwei reviewed Mar 30, 2022
View reviewed changes
Copy link
Contributor

@psschwei psschwei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking out loud here, but do we want to potentially include some kind of validation check for both this setting and a custom TBC value (since by keeping the activator in the path, we're more or less setting TBC = -1 and any other value would essentially get overridden)? It could be overkill, but might also be nice to catch any conflicts at the validation stage.

@nak3
Copy link
Contributor Author

nak3 commented Mar 30, 2022

I'm sure the validation would be useful, but the "activator always in the path" solution is temporary state. So I am inclined to consider about the validation later if "activator always in the path" solution cannot be dropped soon.

In addition, I am lack of knowledge about the auto scale... So I'm worried that I can include the validation without any side effect.

@nak3
Copy link
Contributor Author

nak3 commented Mar 31, 2022 •
edited
Loading

@evankanderson @skonto @rhuss bump

evankanderson
evankanderson reviewed Apr 1, 2022
View reviewed changes
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems safe on its own; the worst case is that you have less-good scaling behavior by setting a key in config-network, but you don't end up being broken altogether by it.

One concern about the structure of the code in kpa.go, but otherwise LGTM.

/approve

pkg/reconciler/autoscaling/kpa/kpa.go Outdated Show resolved Hide resolved
@knative-prow
Copy link

knative-prow bot commented Apr 1, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, nak3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 2, 2022
@skonto
Copy link
Contributor

skonto commented Apr 2, 2022

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2022
@nak3 nak3 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2022
@knative-prow knative-prow bot merged commit 21c05dc into knative:main Apr 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psschwei psschwei psschwei left review comments

@evankanderson evankanderson evankanderson left review comments

@rhuss rhuss Awaiting requested review from rhuss

@skonto skonto Awaiting requested review from skonto

Assignees

@skonto skonto

@evankanderson evankanderson

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/autoscale lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nak3 @dprotaso @evankanderson @skonto @psschwei